[lit] Add readfile substitution #158441

boomanaiden154 · 2025-09-13T21:45:02Z

This patch adds a new %{readfile:} substitution to lit. This
is needed for porting a couple of tests to lit's internal shell. These
tests are all using subshells to pass some option to a command are not
feasible to run within the internal shell without this functionality.

Created using spr 1.3.6

llvmbot · 2025-09-13T21:45:33Z

@llvm/pr-subscribers-testing-tools

Author: Aiden Grossman (boomanaiden154)

Changes

This patch adds a new %{readfile:<file name>} substitution to lit. This
is needed for porting a couple of tests to lit's internal shell. These
tests are all using subshells to pass some option to a command are not
feasible to run within the internal shell without this functionality.

Full diff: https://github.com/llvm/llvm-project/pull/158441.diff

10 Files Affected:

(modified) llvm/docs/CommandGuide/lit.rst (+1)
(modified) llvm/test/tools/llvm-cgdata/empty.test (+1)
(modified) llvm/utils/lit/lit/TestRunner.py (+17)
(added) llvm/utils/lit/tests/Inputs/shtest-readfile/.lit_test_times.txt (+1)
(added) llvm/utils/lit/tests/Inputs/shtest-readfile/Output/absolute-paths.txt.tmp (+1)
(added) llvm/utils/lit/tests/Inputs/shtest-readfile/absolute-paths.txt (+6)
(added) llvm/utils/lit/tests/Inputs/shtest-readfile/lit.cfg (+8)
(added) llvm/utils/lit/tests/Inputs/shtest-readfile/relative-paths.txt (+7)
(added) llvm/utils/lit/tests/Inputs/shtest-readfile/two-same-line.txt (+8)
(added) llvm/utils/lit/tests/shtest-readfile.py (+17)

diff --git a/llvm/docs/CommandGuide/lit.rst b/llvm/docs/CommandGuide/lit.rst
index 15c249d8e6d31..359e0c3e81d0e 100644
--- a/llvm/docs/CommandGuide/lit.rst
+++ b/llvm/docs/CommandGuide/lit.rst
@@ -664,6 +664,7 @@ TestRunner.py:
                          Otherwise, %t but with a single leading ``/`` removed.
  %:T                     On Windows, %/T but a ``:`` is removed if its the second character.
                          Otherwise, %T but with a single leading ``/`` removed.
+ %{readfile:<filename>}  Reads the file specified.
  ======================= ==============
 
 Other substitutions are provided that are variations on this base set and
diff --git a/llvm/test/tools/llvm-cgdata/empty.test b/llvm/test/tools/llvm-cgdata/empty.test
index 52d0dfb87623f..7e42db5ed8512 100644
--- a/llvm/test/tools/llvm-cgdata/empty.test
+++ b/llvm/test/tools/llvm-cgdata/empty.test
@@ -35,3 +35,4 @@ RUN: printf '\000\000\000\000' >> %t_header.cgdata
 RUN: printf '\040\000\000\000\000\000\000\000' >> %t_header.cgdata
 RUN: printf '\040\000\000\000\000\000\000\000' >> %t_header.cgdata
 RUN: diff %t_header.cgdata %t_emptyheader.cgdata
+RUN: echo %{readfile:/tmp/test} > /tmp/test
diff --git a/llvm/utils/lit/lit/TestRunner.py b/llvm/utils/lit/lit/TestRunner.py
index 90c2c6479b004..8d565e8bad53a 100644
--- a/llvm/utils/lit/lit/TestRunner.py
+++ b/llvm/utils/lit/lit/TestRunner.py
@@ -719,6 +719,20 @@ def processRedirects(cmd, stdin_source, cmd_shenv, opened_files):
 
     return std_fds
 
+def _expandLateSubstitutions(arguments, cwd):
+    for i, arg in enumerate(arguments):
+        if not isinstance(arg, str):
+            continue
+        def _replaceReadFile(match):
+            filePath = match.group(1)
+            if not os.path.isabs(filePath):
+                filePath = os.path.join(cwd, filePath)
+            with open(filePath) as fileHandle:
+                return fileHandle.read()
+        
+        arguments[i] = re.sub(r"%{readfile:([^}]*)}", _replaceReadFile, arg)
+    
+    return arguments
 
 def _executeShCmd(cmd, shenv, results, timeoutHelper):
     if timeoutHelper.timeoutReached():
@@ -834,6 +848,9 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
         # Ensure args[0] is hashable.
         args[0] = expand_glob(args[0], cmd_shenv.cwd)[0]
 
+        # Expand all late substitutions
+        args = _expandLateSubstitutions(args, cmd_shenv.cwd)
+
         inproc_builtin = inproc_builtins.get(args[0], None)
         if inproc_builtin and (args[0] != "echo" or len(cmd.commands) == 1):
             # env calling an in-process builtin is useless, so we take the safe
diff --git a/llvm/utils/lit/tests/Inputs/shtest-readfile/.lit_test_times.txt b/llvm/utils/lit/tests/Inputs/shtest-readfile/.lit_test_times.txt
new file mode 100644
index 0000000000000..9802a6bcd406a
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-readfile/.lit_test_times.txt
@@ -0,0 +1 @@
+-4.514933e-03 absolute-paths.txt
diff --git a/llvm/utils/lit/tests/Inputs/shtest-readfile/Output/absolute-paths.txt.tmp b/llvm/utils/lit/tests/Inputs/shtest-readfile/Output/absolute-paths.txt.tmp
new file mode 100644
index 0000000000000..b6fc4c620b67d
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-readfile/Output/absolute-paths.txt.tmp
@@ -0,0 +1 @@
+hello
\ No newline at end of file
diff --git a/llvm/utils/lit/tests/Inputs/shtest-readfile/absolute-paths.txt b/llvm/utils/lit/tests/Inputs/shtest-readfile/absolute-paths.txt
new file mode 100644
index 0000000000000..4246064cf7bfc
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-readfile/absolute-paths.txt
@@ -0,0 +1,6 @@
+## Tests that readfile works with absolute paths
+# RUN: echo -n "hello" > %t
+# RUN: echo %{readfile:%t}
+
+## Fail the test so we can assert on the output.
+# RUN: not echo return
diff --git a/llvm/utils/lit/tests/Inputs/shtest-readfile/lit.cfg b/llvm/utils/lit/tests/Inputs/shtest-readfile/lit.cfg
new file mode 100644
index 0000000000000..25651f2cd4832
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-readfile/lit.cfg
@@ -0,0 +1,8 @@
+import lit.formats
+
+config.name = "shtest-readfile"
+config.suffixes = [".txt"]
+config.test_format = lit.formats.ShTest(execute_external=False)
+config.test_source_root = None
+config.test_exec_root = None
+config.substitutions.append(("%{python}", '"%s"' % (sys.executable)))
diff --git a/llvm/utils/lit/tests/Inputs/shtest-readfile/relative-paths.txt b/llvm/utils/lit/tests/Inputs/shtest-readfile/relative-paths.txt
new file mode 100644
index 0000000000000..3d203d411379d
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-readfile/relative-paths.txt
@@ -0,0 +1,7 @@
+## Tests that readfile works with relative paths
+# RUN: mkdir -p rel_path_test_folder
+# RUN: echo -n "hello" > rel_path_test_folder/test_file
+# RUN: echo %{readfile:rel_path_test_folder/test_file}
+
+## Fail the test so we can assert on the output.
+# RUN: not echo return
diff --git a/llvm/utils/lit/tests/Inputs/shtest-readfile/two-same-line.txt b/llvm/utils/lit/tests/Inputs/shtest-readfile/two-same-line.txt
new file mode 100644
index 0000000000000..6855d27d66d35
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-readfile/two-same-line.txt
@@ -0,0 +1,8 @@
+## Tests that readfile works with two substitutions on the same line to ensure the
+## regular expressions are setup correctly.
+# RUN: echo -n "hello" > %t.1
+# RUN: echo -n "bye" > %t.2
+# RUN: echo %{readfile:%t.1} %{readfile:%t.2}
+
+## Fail the test so we can assert on the output.
+# RUN: not echo return
diff --git a/llvm/utils/lit/tests/shtest-readfile.py b/llvm/utils/lit/tests/shtest-readfile.py
new file mode 100644
index 0000000000000..8ba8e53b32966
--- /dev/null
+++ b/llvm/utils/lit/tests/shtest-readfile.py
@@ -0,0 +1,17 @@
+## Tests the readfile substitution
+
+# RUN: not %{lit} -a -v %{inputs}/shtest-readfile | FileCheck -match-full-lines %s
+
+# CHECK: -- Testing: 3 tests{{.*}}
+
+# CHECK-LABEL: FAIL: shtest-readfile :: absolute-paths.txt ({{[^)]*}})
+# CHECK: echo hello
+# CHECK: # executed command: echo '%{readfile:{{.*}}}'
+
+# CHECK-LABEL: FAIL: shtest-readfile :: relative-paths.txt ({{[^)]*}})
+# CHECK: echo hello
+# CHECK: # executed command: echo '%{readfile:rel_path_test_folder/test_file}'
+
+# CHECK-LABEL: FAIL: shtest-readfile :: two-same-line.txt ({{[^)]*}})
+# CHECK: echo hello bye
+# CHECK: # executed command: echo '%{readfile:{{.*}}.1}' '%{readfile:{{.*}}.2}'

github-actions · 2025-09-13T21:48:58Z

✅ With the latest revision this PR passed the Python code formatter.

Created using spr 1.3.6

cmtice · 2025-09-13T22:31:26Z

llvm/utils/lit/lit/TestRunner.py

+            filePath = match.group(1)
+            if not os.path.isabs(filePath):
+                filePath = os.path.join(cwd, filePath)
+            with open(filePath) as fileHandle:


Shouldn't you verify that the file exists before trying to open & read it?

And fail gracefully otherwise.

That wouldn't be very pythonic. try/except would be more idiomatic, if we want a more graceful failure.

If we were going to try/catch, we would end up throwing an internal shell error without about the same amount of information. I think this is fine to leave as is, but can change it up if desired.

Whether you use try/catch or something else, I still think you should verify the file exists before trying to open it.

We certainly should have a test that covers the case of a missing file. lit should also treat such a case as a FAIL test and not some other failure mode. I don't know whether that requires any particular handling.

To get this to be marked as a failure instead of unresolved we need to propagate it up as a InternalShellError. I've made the necessary modifications and added a test for this case. Please take a look.

jh7370 · 2025-09-15T10:03:39Z

llvm/utils/lit/lit/TestRunner.py

+            filePath = match.group(1)
+            if not os.path.isabs(filePath):
+                filePath = os.path.join(cwd, filePath)
+            with open(filePath) as fileHandle:


That wouldn't be very pythonic. try/except would be more idiomatic, if we want a more graceful failure.

jh7370 · 2025-09-15T10:03:49Z

llvm/utils/lit/lit/TestRunner.py

        # Ensure args[0] is hashable.
        args[0] = expand_glob(args[0], cmd_shenv.cwd)[0]

+        # Expand all late substitutions


Suggested change

# Expand all late substitutions

# Expand all late substitutions.

jh7370 · 2025-09-15T10:04:27Z

llvm/test/tools/llvm-cgdata/empty.test

 RUN: printf '\040\000\000\000\000\000\000\000' >> %t_header.cgdata
 RUN: printf '\040\000\000\000\000\000\000\000' >> %t_header.cgdata
 RUN: diff %t_header.cgdata %t_emptyheader.cgdata
+RUN: echo %{readfile:/tmp/test} > /tmp/test


What's this change for?

Debugging. Looks like I didn't review my PR diff closely enough before requesting review. Sorry for the noise here.

jh7370 · 2025-09-15T10:05:06Z

llvm/utils/lit/tests/Inputs/shtest-readfile/absolute-paths.txt

@@ -0,0 +1,6 @@
+## Tests that readfile works with absolute paths


Suggested change

## Tests that readfile works with absolute paths

## Tests that readfile works with absolute paths.

jh7370 · 2025-09-15T10:05:54Z

llvm/utils/lit/tests/Inputs/shtest-readfile/lit.cfg

+config.test_format = lit.formats.ShTest(execute_external=False)
+config.test_source_root = None
+config.test_exec_root = None
+config.substitutions.append(("%{python}", '"%s"' % (sys.executable)))


Is this needed?

Not for these tests. I cargo-culted this from the ulimit tests. Removed.

jh7370 · 2025-09-15T10:06:23Z

llvm/utils/lit/tests/Inputs/shtest-readfile/relative-paths.txt

@@ -0,0 +1,7 @@
+## Tests that readfile works with relative paths


Suggested change

## Tests that readfile works with relative paths

## Tests that readfile works with relative paths.

jh7370 · 2025-09-15T10:07:10Z

llvm/utils/lit/tests/Inputs/shtest-readfile/two-same-line.txt

@@ -0,0 +1,8 @@
+## Tests that readfile works with two substitutions on the same line to ensure the
+## regular expressions are setup correctly.


Suggested change

## regular expressions are setup correctly.

## regular expressions are set up correctly.

("setup" -> noun; "set up" -> verb or adjective)
/end grammar pedantry

jh7370 · 2025-09-15T10:07:37Z

llvm/utils/lit/tests/shtest-readfile.py

@@ -0,0 +1,17 @@
+## Tests the readfile substitution


Suggested change

## Tests the readfile substitution

## Tests the readfile substitution.

jh7370 · 2025-09-15T10:11:07Z

llvm/utils/lit/tests/shtest-readfile.py

+
+# CHECK-LABEL: FAIL: shtest-readfile :: absolute-paths.txt ({{[^)]*}})
+# CHECK: echo hello
+# CHECK: # executed command: echo '%{readfile:{{.*}}}'


Could you use FileCheck's -D option or similar to allow you to more precisely match the file name mentioned here?

Created using spr 1.3.6

arichardson · 2025-09-16T05:44:55Z

Out of interest could you point me to these tests? Are they just doing something like echo $(cat /some/file) or something more complicated. Adding this substitution sounds good to me since it's much easier than supporting "`pwd`" or "$(pwd)".

boomanaiden154 · 2025-09-16T13:10:16Z

Out of interest could you point me to these tests? Are they just doing something like echo $(cat /some/file) or something more complicated. Adding this substitution sounds good to me since it's much easier than supporting "pwd" or "$(pwd)".

They're usually a bit more complicated. There are a bit under 10 tests in clang that need this in #158446. There are maybe a dozen or so compiler-rt tests. For clang it's usually things like getting the default target triple or manipulating resource directories.

Created using spr 1.3.6

arichardson · 2025-09-17T20:38:14Z

Just saw your other PR #158441, I wonder if the other alternative would be to add limited support for $() - initially only supporting cat and emitting an error otherwise?

boomanaiden154 · 2025-09-17T21:22:02Z

Just saw your other PR #158441, I wonder if the other alternative would be to add limited support for $() - initially only supporting cat and emitting an error otherwise?

I think it's best if we don't implement subshells in any form. I guess it could be implemented in a pretty similar manner to %{readfile:<filename>}, but I think keeping the syntax closer to existing lit substitutions makes things a bit more friendly to people writing tests. If we want to support $(cat <filename>) syntax we quickly run into more parsing issues, like needing to deal with quoted strings that we do not here. Not super difficult to handle, but I would rather not if we don't have to.

The thinking is also that the extra complexity in #158441 doesn't really matter because once we have migrated all the test suites over to the internal shell, we can then drop all of the code supporting the external shell. Maybe with some phase out period to give time for downstreams, but it should be going away eventually.

arichardson · 2025-09-17T21:35:24Z

Just saw your other PR #158441, I wonder if the other alternative would be to add limited support for $() - initially only supporting cat and emitting an error otherwise?

I think it's best if we don't implement subshells in any form. I guess it could be implemented in a pretty similar manner to %{readfile:<filename>}, but I think keeping the syntax closer to existing lit substitutions makes things a bit more friendly to people writing tests. If we want to support $(cat <filename>) syntax we quickly run into more parsing issues, like needing to deal with quoted strings that we do not here. Not super difficult to handle, but I would rather not if we don't have to.

The thinking is also that the extra complexity in #158441 doesn't really matter because once we have migrated all the test suites over to the internal shell, we can then drop all of the code supporting the external shell. Maybe with some phase out period to give time for downstreams, but it should be going away eventually.

If the external shell is going away soon then I have no concerns about adding the extra complexity temporarily.

jh7370

Windows pre-merge check is failing the shtest-readfile test.

llvm/utils/lit/lit/TestRunner.py

Created using spr 1.3.6

jh7370

LGTM.

This patch adds a new %{readfile:<file name>} substitution to lit. This is needed for porting a couple of tests to lit's internal shell. These tests are all using subshells to pass some option to a command are not feasible to run within the internal shell without this functionality. Reviewers: petrhosek, jh7370, ilovepi, cmtice Reviewed By: jh7370, cmtice Pull Request: llvm/llvm-project#158441

[𝘀𝗽𝗿] initial version

98b4330

Created using spr 1.3.6

llvmbot added llvm-lit testing-tools labels Sep 13, 2025

boomanaiden154 requested review from petrhosek, ilovepi, cmtice and jh7370 September 13, 2025 21:45

boomanaiden154 added 2 commits September 13, 2025 21:51

formatting

7901d97

Created using spr 1.3.6

formatting

82bf25b

Created using spr 1.3.6

cmtice reviewed Sep 13, 2025

View reviewed changes

jh7370 reviewed Sep 15, 2025

View reviewed changes

feedback

ff3041a

Created using spr 1.3.6

boomanaiden154 requested a review from jh7370 September 16, 2025 03:45

boomanaiden154 added 3 commits September 17, 2025 16:18

feedback

1599e05

Created using spr 1.3.6

Remove print

c1d1b84

Created using spr 1.3.6

remove unused variable

77fccd4

Created using spr 1.3.6

cmtice approved these changes Sep 17, 2025

View reviewed changes

jh7370 reviewed Sep 18, 2025

View reviewed changes

llvm/utils/lit/lit/TestRunner.py Outdated Show resolved Hide resolved

fix Windows CI

c547d48

Created using spr 1.3.6

boomanaiden154 requested a review from jh7370 September 19, 2025 05:13

jh7370 approved these changes Sep 19, 2025

View reviewed changes

boomanaiden154 merged commit 75dba8e into main Sep 19, 2025
10 checks passed

boomanaiden154 deleted the users/boomanaiden154/lit-add-readfile-substitution branch September 19, 2025 15:24

	# Expand all late substitutions
	# Expand all late substitutions.

		@@ -0,0 +1,6 @@
		## Tests that readfile works with absolute paths

		@@ -0,0 +1,7 @@
		## Tests that readfile works with relative paths

		@@ -0,0 +1,8 @@
		## Tests that readfile works with two substitutions on the same line to ensure the
		## regular expressions are setup correctly.

	## regular expressions are setup correctly.
	## regular expressions are set up correctly.

	## Tests the readfile substitution
	## Tests the readfile substitution.

[lit] Add readfile substitution #158441

[lit] Add readfile substitution #158441

Uh oh!

Conversation

boomanaiden154 commented Sep 13, 2025

Uh oh!

llvmbot commented Sep 13, 2025

Uh oh!

github-actions bot commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arichardson commented Sep 16, 2025

Uh oh!

boomanaiden154 commented Sep 16, 2025

Uh oh!

arichardson commented Sep 17, 2025

Uh oh!

boomanaiden154 commented Sep 17, 2025

Uh oh!

arichardson commented Sep 17, 2025

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jh7370 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 13, 2025 •

edited

Loading